Skip to content

Conversation

@daniellansun
Copy link
Contributor

@daniellansun daniellansun requested a review from Copilot October 26, 2025 14:58
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements ES7-like async/await functionality for Groovy, introducing new language keywords async and await along with supporting infrastructure for asynchronous programming.

Key Changes:

  • Added async and await as new language keywords in the Groovy parser and lexer
  • Implemented Promise-based asynchronous execution model with Promise, SimplePromise, and helper classes
  • Created AST transformation to convert async methods into Promise-returning methods

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/antlr/GroovyLexer.g4 Adds ASYNC and AWAIT tokens to the lexer
src/antlr/GroovyParser.g4 Adds async modifier and await expression rules to grammar
src/main/java/org/apache/groovy/parser/antlr4/AstBuilder.java Implements AST building for async/await expressions and method modifiers
src/main/java/org/apache/groovy/parser/antlr4/ModifierManager.java Adds ASYNC to invalid modifiers list for constructors
src/main/java/org/codehaus/groovy/ast/ModifierNode.java Registers ASYNC as virtual modifier with no JVM flag
src/main/java/org/codehaus/groovy/transform/AsyncASTTransformation.java Implements transformation of async methods to return Promises
src/main/java/groovy/transform/Async.java Defines @async annotation for marking async methods
src/main/java/groovy/util/concurrent/async/*.java Core async infrastructure including Promise, Awaitable, AsyncHelper, and exception types
src/test/groovy/**/*Test.groovy Comprehensive test coverage for async/await functionality
src/test-resources/core/AsyncAwait_01x.groovy Parser test case for async/await syntax

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

* result.
*
* @return the computed result
* @throws AwaitException if the computation was cancelled or completed
Copy link

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation for the await() method's exception is incomplete. It should state 'if the computation was cancelled or completed exceptionally' to clarify that exceptions are thrown on error conditions, not successful completion.

Suggested change
* @throws AwaitException if the computation was cancelled or completed
* @throws AwaitException if the computation was cancelled or completed exceptionally

Copilot uses AI. Check for mistakes.
@codecov-commenter
Copy link

codecov-commenter commented Oct 26, 2025

Codecov Report

❌ Patch coverage is 75.82418% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.0320%. Comparing base (4e0dc1a) to head (5e7e868).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
...va/groovy/util/concurrent/async/SimplePromise.java 83.3333% 16 Missing ⚠️
...java/groovy/util/concurrent/async/AsyncHelper.java 70.0000% 5 Missing and 1 partial ⚠️
...a/groovy/util/concurrent/async/AwaitException.java 25.0000% 6 Missing ⚠️
...ain/java/groovy/util/concurrent/async/Promise.java 0.0000% 5 Missing ⚠️
...a/groovy/util/concurrent/async/StageAwaitable.java 0.0000% 5 Missing ⚠️
...ehaus/groovy/transform/AsyncASTTransformation.java 80.9524% 2 Missing and 2 partials ⚠️
...va/org/apache/groovy/parser/antlr4/AstBuilder.java 88.8889% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##               master      #2324        +/-   ##
==================================================
+ Coverage     67.0088%   67.0320%   +0.0231%     
- Complexity      29326      29416        +90     
==================================================
  Files            1382       1388         +6     
  Lines          116604     116804       +200     
  Branches        20427      20446        +19     
==================================================
+ Hits            78135      78296       +161     
- Misses          32036      32069        +33     
- Partials         6433       6439         +6     
Files with missing lines Coverage Δ
...g/apache/groovy/parser/antlr4/ModifierManager.java 93.4210% <100.0000%> (ø)
...ain/java/org/codehaus/groovy/ast/ModifierNode.java 77.7778% <100.0000%> (+0.4193%) ⬆️
...va/org/codehaus/groovy/ast/tools/GeneralUtils.java 86.0412% <100.0000%> (+0.1943%) ⬆️
...va/org/apache/groovy/parser/antlr4/AstBuilder.java 86.6667% <88.8889%> (+0.0444%) ⬆️
...ehaus/groovy/transform/AsyncASTTransformation.java 80.9524% <80.9524%> (ø)
...ain/java/groovy/util/concurrent/async/Promise.java 0.0000% <0.0000%> (ø)
...a/groovy/util/concurrent/async/StageAwaitable.java 0.0000% <0.0000%> (ø)
...java/groovy/util/concurrent/async/AsyncHelper.java 70.0000% <70.0000%> (ø)
...a/groovy/util/concurrent/async/AwaitException.java 25.0000% <25.0000%> (ø)
...va/groovy/util/concurrent/async/SimplePromise.java 83.3333% <83.3333%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

*
* @since 6.0.0
*/
public class AsyncHelper {
Copy link

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The system property 'groovy.async.parallelism' is not documented. Consider adding a comment explaining its purpose and valid values.

Suggested change
public class AsyncHelper {
public class AsyncHelper {
/**
* The system property "groovy.async.parallelism" controls the parallelism level
* (number of threads) used for the async thread pool. Valid values are positive integers.
* If not set, the default is (number of available processors + 1).
* Setting this value too high may lead to resource exhaustion; too low may reduce concurrency.
*/

Copilot uses AI. Check for mistakes.
public class AsyncHelper {
private static final int PARALLELISM = SystemUtil.getIntegerSafe("groovy.async.parallelism", Runtime.getRuntime().availableProcessors() + 1);
private static final Executor DEFAULT_EXECUTOR;
private static int seq;
Copy link

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The static field seq is accessed from multiple threads without synchronization in the thread factory. Use AtomicInteger instead to prevent race conditions.

Copilot uses AI. Check for mistakes.
WHILE : 'while';
YIELD : 'yield';


Copy link

Copilot AI Oct 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Extra blank line added at line 490. Remove this unnecessary whitespace to maintain consistent formatting.

Suggested change

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +158 to +161
import static org.apache.groovy.parser.antlr4.GroovyParser.ADD;
import static org.apache.groovy.parser.antlr4.GroovyParser.ARROW;
import static org.apache.groovy.parser.antlr4.GroovyParser.AS;
import static org.apache.groovy.parser.antlr4.GroovyParser.ASYNC;
Copy link

Copilot AI Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The wildcard import at line 157 has been replaced with explicit imports. While this improves clarity by showing exactly which symbols are used, it significantly increases the number of import statements (158-362). Consider whether this trade-off between explicitness and verbosity aligns with the project's style guidelines.

Copilot uses AI. Check for mistakes.
@daniellansun
Copy link
Contributor Author

@codecov-commenter

@asf-gitbox-commits asf-gitbox-commits force-pushed the GROOVY-9381 branch 3 times, most recently from 9ab2dee to 448d92a Compare October 27, 2025 18:10
@daniellansun daniellansun requested a review from Copilot October 27, 2025 18:10

This comment was marked as duplicate.

@asf-gitbox-commits asf-gitbox-commits force-pushed the GROOVY-9381 branch 2 times, most recently from dba9bf8 to e8d5ac1 Compare October 27, 2025 18:48
@daniellansun daniellansun requested a review from Copilot October 27, 2025 18:49

This comment was marked as duplicate.

@asf-gitbox-commits asf-gitbox-commits force-pushed the GROOVY-9381 branch 4 times, most recently from 71ef10c to 55d94fb Compare October 28, 2025 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants